Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(node-tracer): use AsyncLocalStorageContextManager by default starting Node 14.8 #1511 #1525

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

vmarchaud
Copy link
Member

@vmarchaud vmarchaud commented Sep 13, 2020

Even though AsyncLocalStorage was added before (and its have been backported in 12.x), there was some important bug fixes released for 14.8 (see https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V14.md#commits-3). Specially those one: nodejs/node#34573 and nodejs/node#34573

Fixes #1511

@vmarchaud vmarchaud self-assigned this Sep 13, 2020
@vmarchaud vmarchaud added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 13, 2020
@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #1525 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
- Coverage   92.92%   92.91%   -0.02%     
==========================================
  Files         156      156              
  Lines        4851     4855       +4     
  Branches      978      980       +2     
==========================================
+ Hits         4508     4511       +3     
- Misses        343      344       +1     
Impacted Files Coverage Δ
...kages/opentelemetry-node/src/NodeTracerProvider.ts 96.55% <83.33%> (-3.45%) ⬇️

import { NoopContextManager } from '@opentelemetry/context-base';
import { CompositePropagator } from '@opentelemetry/core';
import * as assert from 'assert';
import { NodeTracerProvider } from '../src';
import * as semver from 'semver';

const DefaultContextManager = semver.gte(process.version, '14.8.0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rather check if AsyncLocalStorageContextManager is available (feature detection) instead of checking the version using semver. If you already require that it means it will be undefined for lower versions

const DefaultContextManager = AsyncLocalStorageContextManager || AsyncHooksContextManager

should be enough ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the description he mentioned that there is a bug in versions before 14.8. Feature detection would still detect that local storage is available, but he suggests we would want to use the async hooks version anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sry haven't spotted that

@johanneswuerbach
Copy link
Contributor

Any reason why Node 12 is excluded? We run AsyncLocalStorageContextManager successfully on Node 12 for a couple of months and the performance improvements should be comparable to the AsyncHooksContextManager.

Node 12 will be supported for ~1,5 years so I guess it could be worth it.

@vmarchaud
Copy link
Member Author

vmarchaud commented Sep 15, 2020

Any reason why Node 12 is excluded? We run AsyncLocalStorageContextManager successfully on Node 12 for a couple of months and the performance improvements should be comparable to the AsyncHooksContextManager.

@johanneswuerbach I stated why i didn't include it on the PR description:

Even though AsyncLocalStorage was added before (and its have been backported in 12.x), there was some important bug fixes released for 14.8 (see https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V14.md#commits-3). Specially those one: nodejs/node#34573 and nodejs/node#34573

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

@johanneswuerbach
Copy link
Contributor

johanneswuerbach commented Sep 16, 2020

Sorry I had assumed nodejs/node#34573 was already back-ported to Node 12, but you are right it isn't :-( Seeing that I would agree with Node 14+ only for now.

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

What I'm not sure about is how long I would support a specific minor/patch version of a major release line.

Should there be 1 month post release, 3 months, forever? I would expect people to follow minors in their major lines relatively closely to also rollout security fixes quickly (e.g. the just release 14.11.0 fixes a critical and a high vulnerability), but I'm not sure how realistic this is and whether it is okay for this project to require user to be on a recent minor version.

If different minor versions have require different behaviour than those should also be covered in CI, which would increase test runtime and costs.

Overall I guess it depends more on the project support agreement then on me, I'm fine with tracking the latest of each major line, but we also invested time and effort in my company that we can and do adopt new minors within 24-48h post release.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vmarchaud
Copy link
Member Author

What I'm not sure about is how long I would support a specific minor/patch version of a major release line.

Not sure to follow here, we would just support all versions starting the 14.8.0 ?

@dyladan
Copy link
Member

dyladan commented Sep 21, 2020

Sorry I had assumed nodejs/node#34573 was already back-ported to Node 12, but you are right it isn't :-( Seeing that I would agree with Node 14+ only for now.

I'm not confortable with enabling it it by default if there are bugs in the wild, WDYT ?

What I'm not sure about is how long I would support a specific minor/patch version of a major release line.

Should there be 1 month post release, 3 months, forever? I would expect people to follow minors in their major lines relatively closely to also rollout security fixes quickly (e.g. the just release 14.11.0 fixes a critical and a high vulnerability), but I'm not sure how realistic this is and whether it is okay for this project to require user to be on a recent minor version.

If different minor versions have require different behaviour than those should also be covered in CI, which would increase test runtime and costs.

Overall I guess it depends more on the project support agreement then on me, I'm fine with tracking the latest of each major line, but we also invested time and effort in my company that we can and do adopt new minors within 24-48h post release.

As stated by @vmarchaud, this is a minimum version not the only minor version supported. Future minor versions and even future major versions will still use the new manager.

@johanneswuerbach
Copy link
Contributor

johanneswuerbach commented Sep 21, 2020

Looks like the PR that broke v14 was actually not back-ported to v12 yet, so v12 shouldn't be broken. Nevertheless I requested a fix to be back-ported to v12 nodejs/node#35287

@johanneswuerbach
Copy link
Contributor

johanneswuerbach commented Sep 22, 2020

Looks like >= 12.18.3 has a bug-free AsyncLocalStorage implementation nodejs/node#35287 (comment) and 12.19 will mainly get some performance fixes nodejs/node#35287 (comment)

Would it make sense to also enable AsyncLocalStorageContextManager for >= 12.18.3 then?

@dyladan
Copy link
Member

dyladan commented Sep 23, 2020

Discussed at SIG today, enabling for 14.8+ only is fine. It is a simple rule that is easy to document. Users on 12 who need the performance boost can always enable whatever context manager they want manually if they'd like.

@dyladan dyladan merged commit aac3f56 into open-telemetry:master Oct 1, 2020
@dyladan dyladan added the enhancement New feature or request label Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable AsyncLocalStorageContextManager by default in some cases
6 participants